-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return &Arc
reference to inner trait object
#11103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the CI is failing -- I think running |
Thank you @alamb ! I've fixed the code formatting issue. |
@@ -236,13 +236,14 @@ mod tests { | |||
fn setup_context() -> (SessionContext, Arc<dyn SchemaProvider>) { | |||
let mut ctx = SessionContext::new(); | |||
ctx.register_catalog_list(Arc::new(DynamicFileCatalog::new( | |||
ctx.state().catalog_list(), | |||
ctx.state().catalog_list().clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I see .clone()
Im now thinking what if we comment it the similar way as .unwrap()
back in the day. Like say clone is cheap here because it is Arc::clone so only reference gets cloned.... thats just an idea, its too cumbersome to make it happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we do in InfluxDB is use this pattern to make it clear
ctx.state().catalog_list().clone(), | |
Arc::clone(ctx.state().catalog_list()) |
to make it explicit.
There is a clippy lint https://rust-lang.github.io/rust-clippy/v0.0.212/#clone_on_ref_ptr we could turn on in datafusion to enable this
Here is how it is done in influxdb:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea, let's do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #11143 to track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @linhr this pattern makes a lot of sense to me
* Return `&Arc` reference to inner trait object * Format code
Which issue does this PR close?
Closes #11100.
Rationale for this change
It's better to return
&Arc
reference to inner trait object for&self
methods. This makes it easier to work with Rust lifetime rules when we need to cast trait object to concrete reference types. Please refer to the corresponding issue for an example.What changes are included in this PR?
This PR updates the return type for few methods in
ScalarUDF
,AggregateUDF
,WindowUDF
, andSessionState
.Are these changes tested?
The changes are covered by existing tests.
Are there any user-facing changes?
Yes, these are breaking changes to the public API. The changes should be small, since the user can simply call
.clone()
on&Arc
if needed.